Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added clarification of notifications to be sent and added StatusInfo "DELETE_REQUESTED" to addres #241 #258

Merged
merged 7 commits into from
Feb 1, 2024
Merged

Added clarification of notifications to be sent and added StatusInfo "DELETE_REQUESTED" to addres #241 #258

merged 7 commits into from
Feb 1, 2024

Conversation

hdamker
Copy link
Collaborator

@hdamker hdamker commented Jan 23, 2024

What type of PR is this?

Add one of the following kinds:

  • documentation
  • correction

What this PR does / why we need it:

Adding clarification that notification will be sent for all changes of qosStatus and what happens when qosStatus change from AVAILABLE to UNAVAILABLE

Which issue(s) this PR fixes:

Fixes #241 and #254

Special notes for reviewers:

As notification should be sent in all QosStatus changes, there was the need to add the StatusInfo "DELETE_REQUESTED".

Changelog input

Clarified in documentation that notifications will be sent for all changes of QosStatus, even if initiated by the client.
Clarified in documentation that the QoS session resource what will happen when qosStatus changes from 'AVAILABLE' to 'UNAVAILABLE' due to 'NETWORK_TERMINATED'

hdamker and others added 2 commits January 23, 2024 13:44
Co-authored-by: Shilpa Padgaonkar <77152136+shilpa-padgaonkar@users.noreply.github.com>
@hdamker hdamker added bug Something isn't working documentation Improvements or additions to documentation labels Jan 23, 2024
@jlurien
Copy link
Collaborator

jlurien commented Jan 23, 2024

In my understanding, the PR is not directly addressing the issue description:

  • after the qosStatus changes at step 11 from "Available" to "Unavailable" (not because of duration expiry), we do not terminate the QoS session. This fact is not specified explicitly in the current API documentation clearly. Could we extend the documentation with this point so that it is also clear to the developers. This will then make the developers aware that they need to explicitly invoke a delete session. after getting a notification that qosStatus is unavailable.

We are not indicating that the UNAVAILABLE notification does not delete the session automatically. The PR addresses that notifications will be sent even after the session is deleted by the user. But in case of unavailability due to network terminated, this is another case.

@hdamker hdamker requested a review from maxl2287 January 25, 2024 16:56
@hdamker
Copy link
Collaborator Author

hdamker commented Jan 25, 2024

@jlurien You are right. I have only addressed yet part of the derived conclusion from our discussion in the call on Dec 15th (added to the issue):

Design target is to allow developers to decide between notifications and polling, both methods should be sufficient to get all important information

This design target was the reason to not delete the QoD session resource implicitly after sending the QOS_STATUS_CHANGED event to UNAVAILABLE if the statusInfo is NETWORK_TERMINATED: a client not receiving notifications wouldn't be ever able to see this statusInfo if we would delete the session immediately.

But we also discussed that we can't expect that a client will always send an explicit delete for such a session resource - it's not even clear if a client would recognise the situation, e.g. if it was set up for a specific duration and the client hasn't provided a callback.

We can also not rely on the expiration of the defined session duration, as this would be done by the network and the related network session is already terminated / not anymore available

The discussed solution was therefore to keep the session resource for a fixed time span after it got unavailable due to "NETWORK_TERMINATED". E.g. 6 minutes to be sufficient for clients which poll each 5 minutes the status.

I will add the description of this discussed solution.

We haven't discussed the behavior for "DURATION_EXPIRED" - my understanding is that in this case the session resource will be implicitly also deleted as this is the expected behavior and no longer exist after this event. Any other opinion?

Added behavior in case of `NETWORK_TERMINATED` and the need to delete a session explicitly after such event with the time period of 360 seconds before a new QoS session for the same device and flow can be created.

In addition clarified that there will be no notification event if a session with qosStatus `UNAVAILABLE` will be deleted.
@hdamker
Copy link
Collaborator Author

hdamker commented Jan 25, 2024

Added the behavior in case of NETWORK_TERMINATED and the need to delete a session explicitly after such event within the time period of 360 seconds before a new QoS session for the same device and flow can be created.

Note: the 360 seconds are just an initial proposal (to allow a poll period up to 300 seconds).

In addition clarified that there will be no notification event if a session with qosStatus UNAVAILABLE will be deleted.

@hdamker
Copy link
Collaborator Author

hdamker commented Jan 25, 2024

@jlurien @shilpa-padgaonkar @maxl2287 @RandyLevensalor @eric-murray would be great if you could review this PR (again) before the QoD call tomorrow. It would be good if we can decide tomorrow about a second release candidate of v0.10.0 which includes this PR.

Copy link
Collaborator

@shilpa-padgaonkar shilpa-padgaonkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/LGTM

Copy link
Collaborator

@RandyLevensalor RandyLevensalor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see comments below regarding the notes below.

code/API_definitions/qod-api.yaml Outdated Show resolved Hide resolved
code/API_definitions/qod-api.yaml Outdated Show resolved Hide resolved
code/API_definitions/qod-api.yaml Outdated Show resolved Hide resolved
@maxl2287
Copy link
Collaborator

LGTM

Added clarification that the automatic deletion of session resources after `NETWORK_TERMINATED` will happen at earliest after 360 seconds but could happen also later.
@hdamker
Copy link
Collaborator Author

hdamker commented Jan 27, 2024

As agreed within our call I've added the clarification that the automatic deletion of session resources after NETWORK_TERMINATED will happen at earliest after 360 seconds (but could happen also later).

@jlurien @maxl2287 @RandyLevensalor @eric-murray Please do another review/approval early within the week.

Copy link
Collaborator

@RandyLevensalor RandyLevensalor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hdamker hdamker merged commit ecd649c into camaraproject:main Feb 1, 2024
2 checks passed
@hdamker hdamker deleted the documentation/notifications#241 branch February 1, 2024 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
6 participants